Zend: refactor zend_parse_arg_impl() to return zend_expected_type by Girgias · Pull Request #23052 · php/php-src

Girgias · 2026-08-04T21:53:19Z

This effectively mimics part of what Fast ZPP does and allows us to re-use the fast ZPP error APIs

Further refactorings: #23104 and #23105.

This effectively mimics part of what Fast ZPP does and allows us to re-use the fast ZPP error APIs

kamil-tekiela · 2026-08-07T19:09:07Z

-					if (Z_TYPE_P(arg) == IS_STRING) {
-						zend_spprintf(error, 0, "must not contain any null bytes");
-						return "";
-					} else {
-						return check_null ? "?string" : "string";
-					}


Do I understand correct that this was just so that the user is informed about NUL bytes in their path string? Because the NUL bytes are already handled in zend_str_has_nul_byte but the error message would be generic, right? Now, you handle it with zend_wrong_parameter_type_error.

Yes. This is the purpose of this ZPP specifier, and has always been this way.

You get the same error message either way, fast ZPP only uses zend_wrong_parameter_type_error to generate error messages.

Ok, but one thing I don't understand. Previously if (Z_TYPE_P(arg) == IS_STRING) { was after ZVAL_DEREF(arg) but now in the zend_wrong_parameter_type_error, you have this check but I cannot find any ZVAL_DEREF(arg) up the call stack. Am I missing something?

Do we have a unit test for something like this:

$s = "abc\0def"; $ref = &$s; fopen($ref, 'r');

EDIT: So I asked Claude to explain this to me and it told me that it's not possible for zend_parse_arg/zend_parse_arg_impl to receive a reference, so there is no need for dereference checks. I can't say that I understand why it's in one place and not in the other, but at least it doesn't cause a regression.

I'm not interested in a "review" from Claude.

Just to clarify, to avoid any misunderstanding, the review is mine. I only used Claude to help me understand the code. I am genuinely asking why we have ZVAL_DEREF(arg) but not in the other place.

There is a ZVAL_DEREF() at the top of this function. I don't understand what you are struggling with.

Sorry if I was not clear. Let me try to express my concern better.

On line 841 is a ZVAL_DEREF(arg); which you did not remove. I thought that it was crucial for if (Z_TYPE_P(arg) == IS_STRING) { to work when a reference was passed as an argument. Now, the check is effectively moved out of zend_parse_arg_impl and into zend_wrong_parameter_type_error, which is called directly from zend_parse_arg. On line 282, the check is there again, but this time, there is no ZVAL_DEREF(arg); in zend_wrong_parameter_type_error nor in zend_parse_arg.

I tested, and I don't think anything's broken, so it's probably just me being confused. This is why I asked the question. Shouldn't the ZVAL_DEREF(arg); live in zend_parse_arg or be duplicated in zend_wrong_parameter_type_error? I guess what I am trying to understand is whether ZVAL_DEREF(arg) changes the argument or only the local copy? As I understand it, it only changes the local copy, so IMHO, it probably should be moved from line 841 to line 1077.

I confirm the issue.

It's not an issue for fopen() because the arg is not by-ref, so the VM derefs during argument passing.

However it would be an issue if we had a function taking a path by-ref, as zend_wrong_parameter_type_error() sees a reference in this case. This leads to an error message like:

Argument #1 ($path) must be of type string, string given

I tested by adding a function in ext/zend_test.

diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c index a880c09fc1f..fe670890f7d 100644 --- a/ext/zend_test/test.c +++ b/ext/zend_test/test.c @@ -1969,3 +1969,14 @@ static PHP_FUNCTION(zend_test_gh19792) zend_error(E_WARNING, "a warning"); zend_throw_error(NULL, "an exception"); } + +static PHP_FUNCTION(zend_test_recv_path_by_ref_old_zpp) +{ + zend_string *path; + + if (zend_parse_parameters(ZEND_NUM_ARGS(), "p", &path) == FAILURE) { + RETURN_THROWS(); + } + + RETURN_STR(path); +} diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php index 7c96ea176a8..6c8ffd4ca08 100644 --- a/ext/zend_test/test.stub.php +++ b/ext/zend_test/test.stub.php @@ -392,6 +392,8 @@ function zend_test_uri_parser(string $uri, string $parser): array { } /** @compile-time-eval */ function zend_test_gh19792(): void {} + + function zend_test_recv_path_by_ref_old_zpp(string &$path): void {} } namespace ZendTestNS {

$ php -r '$a = "foo\0bar"; zend_test_recv_path_by_ref_old_zpp($a);' Fatal error: Uncaught TypeError: zend_test_recv_path_by_ref_old_zpp(): Argument #1 ($path) must be of type string, string given in Command line code:1

kamil-tekiela · 2026-08-07T19:27:39Z

<?php
declare(strict_types=1);
get_class_vars(123);

This will throw a different message now, correct? We don't seem to have a test for this. With declare(strict_types=1)

kamil-tekiela · 2026-08-09T22:08:50Z

<?php
declare(strict_types=1);
get_class_vars(123);
This will throw a different message now, correct? We don't seem to have a test for this. With declare(strict_types=1)

Regarding this, my question is why there are no changes in the tests that reflect the different behaviour I observed with that code sample.
Error before:

Argument #1 ($class) must be a valid class name, 123 given

Error after:

Argument #1 ($class) must be of type string, int given

arnaud-lb · 2026-08-10T09:17:40Z

+				if (EXPECTED(zend_parse_arg_func(arg, fci, fcc, check_null, error, c == 'f'))) {
+					ZEND_ASSERT(!*error);


Nit / not for this PR: Both zend_parse_arg_func() and zend_is_callable_at_frame() zero *error. I'm wondering if we could move this responsibility to the caller. In the case of zend_parse_arg_impl(), *error is always NULL before calling zend_parse_arg_func() and zend_is_callable_at_frame().

arnaud-lb · 2026-08-10T09:28:08Z

-					zend_argument_type_error(arg_num, "%s", error);
+				switch (expected_type) {
+					case Z_EXPECTED_OBJECT:
+						/* DO NOT FREE error: it's a pointer to ZSTR_VAL(ce->name) */


Nit / not for this PR: We should unify ownership of error and maybe make it a zend_string*.

github-actions Bot added Category: Engine Extension: bcmath Extension: gmp Extension: mysqli ABI break labels Aug 4, 2026

Girgias force-pushed the 2026-08-old-zpp-use-error-msg branch from feae6eb to c471e54 Compare August 5, 2026 00:59

github-actions Bot added the Extension: pdo_mysql label Aug 5, 2026

Girgias force-pushed the 2026-08-old-zpp-use-error-msg branch from c471e54 to 3ccd8f2 Compare August 7, 2026 17:28

This was referenced Aug 7, 2026

Zend: refactor zend_parse_arg_class() so that it can be reused #23104

Draft

Zend: refactor fast ZPP to only use zend_expected_type #23105

Draft

Girgias marked this pull request as ready for review August 7, 2026 18:21

Girgias requested review from bukka, dstogov and kamil-tekiela as code owners August 7, 2026 18:21

Girgias requested a review from arnaud-lb August 7, 2026 18:21

kamil-tekiela reviewed Aug 7, 2026

View reviewed changes

Comment thread Zend/zend_API.c Outdated

Zend: refactor zend_parse_arg_impl() to return zend_expected_type

750ff30

This effectively mimics part of what Fast ZPP does and allows us to re-use the fast ZPP error APIs

Girgias force-pushed the 2026-08-old-zpp-use-error-msg branch from 3ccd8f2 to 750ff30 Compare August 7, 2026 18:57

kamil-tekiela reviewed Aug 7, 2026

View reviewed changes

arnaud-lb reviewed Aug 10, 2026

View reviewed changes

		if (EXPECTED(zend_parse_arg_func(arg, fci, fcc, check_null, error, c == 'f'))) {
		ZEND_ASSERT(!*error);

Uh oh!

Conversation

Girgias commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kamil-tekiela commented Aug 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Girgias commented Aug 4, 2026 •

edited

Loading

kamil-tekiela Aug 7, 2026 •

edited

Loading

arnaud-lb Aug 10, 2026 •

edited

Loading

kamil-tekiela commented Aug 7, 2026 •

edited

Loading